feat(tls): allow password users to connect over TLS without a client certificate - #1283
Conversation
Setting `tls_client_ca_certificate` makes client certificates mandatory
for every TLS connection, because rustls' default client verifier denies
anonymous clients. On a listener serving both mTLS users (`identity`) and
password users, that leaves the password users unable to negotiate TLS at
all: they get a `certificate_required` alert, so their only working option
is a plaintext connection.
PgDog cannot decide this during the handshake, which completes before the
startup packet carrying `user=` arrives. At that point it has no idea who
is connecting.
Build the verifier with `allow_unauthenticated()` so a certificate is
verified when presented but its absence no longer fails the handshake, and
move the requirement to authentication, where the user is known. A new
per-user `tls_client_certificate_required` defaults to true and is enforced
only when a client CA is configured and the client connected over TLS:
- without a client CA no certificate is ever requested, so requiring one
could never be satisfied;
- plaintext connections stay governed by `tls_client_required`.
Behavior is unchanged at the default. A user connecting over TLS without a
certificate is still rejected, now with `NoClientCertificate` at the auth
layer rather than a TLS alert. mTLS is unaffected: users with `identity`
are still rejected by the existing `tls_identity()` comparison, and an
entry with neither identity nor password still fails closed via
`NoPasswordConfig`. Certificates that are presented are verified against
the CA bundle exactly as before; only the empty-certificate case changes.
Track certificate presence on `Stream` separately from `tls_identity`,
which is `None` both when no certificate was sent and when one was sent
without a dNSName SAN or Subject CN.
Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Extract the four-condition guard into `missing_required_client_certificate` so the logic is stated once and can be exercised directly, and cover the cases Codecov flagged: - a truth table over the guard, including each way it must NOT fire - `NoClientCertificate` is an error and renders its message - plain and dev_null streams report no client certificate Also shorten the doc comments added in the previous commit. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
The unit tests cover the pieces but nothing drives a real handshake through to authentication, because `Client::login` needs a live TLS stream and a populated cluster registry. The mTLS integration suite already has both, so add the two cases this change introduces to it: a password user that opts out connects over TLS without a certificate, and one that doesn't opt out is still rejected. Both share a listener with the existing identity users, which is the arrangement the feature exists to allow. Co-Authored-By: Claude <noreply@anthropic.com>
| /// Only meaningful when a client CA is configured, since otherwise no certificate | ||
| /// is ever requested, and only over TLS, since plaintext is governed by | ||
| /// `tls_client_required`. | ||
| fn missing_required_client_certificate( |
There was a problem hiding this comment.
nit: you're better off passing in the original structs by reference, that way you can be sure the right bools are in the right place.
There was a problem hiding this comment.
Alternatively, you can make it a struct, so it's less likely to make a typo:
struct MissingParams {
client_ca_configured: bool,
is_tls: bool,
// ...
}
levkk
left a comment
There was a problem hiding this comment.
LGTM! Happy to merge as-is, but let me know if you'd want to refactor that 4 boolean method.
Four positional booleans made it possible to transpose `required` and `presented` and silently stop enforcing the requirement, since only `presented` is negated. Named fields make that wrong on its face. The test loses its positional comment and now varies one field at a time from a rejected baseline, so each case states which condition it is exercising. Co-Authored-By: Claude <noreply@anthropic.com>
|
Took the struct suggestion — thanks, it was a real hole rather than just a style point. Because the body is a flat } else if (ClientCertificateCheck {
client_ca_configured,
is_tls: stream.is_tls(),
required: cluster.tls_client_certificate_required(),
presented: stream.tls_client_certificate(),
})
.rejected()
{I went with the params struct over passing The test also reads better now — it varies one field at a time from a rejected baseline, so each case names the condition it's exercising instead of relying on a |
Problem
Setting
tls_client_ca_certificatemakes client certificates mandatory for every TLS connection, because rustls' default client verifier denies anonymous clients. On a listener serving both mTLS users (identity) and password users (password/password_hash), that leaves the password users unable to negotiate TLS at all — they get acertificate_requiredalert, so their only working option is a plaintext connection:PgDog can't resolve this during the handshake, which completes before the startup packet carrying
user=arrives — at that point it has no idea who is connecting. So the choice is currently all-or-nothing for every connection on the listener.Worth noting the current requirement doesn't actually protect those users: they're already reachable unauthenticated over plaintext. It only forces the legitimate ones onto the unencrypted path.
Change
Build the verifier with
allow_unauthenticated()so a certificate is verified when presented but its absence no longer fails the handshake, and move the requirement to authentication, where the user is known.A new per-user
tls_client_certificate_requireddefaults totrueand is enforced only when a client CA is configured and the client connected over TLS:with_no_client_auth), so requiring one could never be satisfied — enforcing it unconditionally would reject every user in every deployment that doesn't configure client CAs;tls_client_required.Backwards compatibility
Behavior is unchanged at the default:
NoClientCertificate)tls_client_requiredThe one visible difference at default is that a certless client is now rejected after the handshake rather than during it, so the error is a Postgres auth failure instead of a TLS alert. That also means slightly more work per rejected connection — happy to gate the verifier change behind a config flag instead if you'd prefer to keep the early rejection.
mTLS is unaffected
Identity enforcement never lived in the handshake —
client/mod.rsalready requiresstream.tls_identity() == Some(identity)for any user with a configured identity, and a certless client getsNone, so it still fails withNoIdentity. An entry with neither identity nor password still fails closed via the existingNoPasswordConfigguard. Certificates that are presented are verified against the CA bundle exactly as before; only the empty-certificate case changes.Testing
cargo fmt --all -- --checkandcargo clippy --all-targets -- -D warningscleancargo test -p pgdog-config— 83 passedclient_cert_verifier_allows_anonymous_clientsassertsoffer_client_auth()stays true whileclient_auth_mandatory()becomes false; two config tests cover the unset and opted-out cases